-
Notifications
You must be signed in to change notification settings - Fork 92
feat(spark): support Hive DDL / Insert operations #518
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(spark): support Hive DDL / Insert operations #518
Conversation
f5f407e to
f65f969
Compare
f65f969 to
8940337
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good. Just a couple of questions from me, and a small suggestion.
| ) | ||
| } | ||
| } | ||
| case WriteOp.INSERT => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice no isHive check here. Might it be possible to arrive here when isHive is not true?
| val bytes = new PlanProtoConverter() | ||
| .toProto(substraitPlan) | ||
| .toByteArray | ||
|
|
||
| // Read it back | ||
| val protoPlan = io.substrait.proto.Plan | ||
| .parseFrom(bytes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's valid but I'm wondering if there is any benefit to doing the work of de/serializing to bytes, rather than just using the protobuf object. Is it exercising anything other than protobuf's ability to de/serialize?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I put all tests through the full roundtrip lifecycle, including down to the proto bytes to ensure that all the visitors within the proto/rel converters have been implemented.
| assertResult(5)(spark.sql("select * from test").count()) | ||
| } | ||
|
|
||
| test("CTAS") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the less expert amongst us, it might be clearer as:
| test("CTAS") { | |
| test("Create table as select") { |
Add support for DdlRel and WriteRel for Hive in Spark Signed-off-by: Andrew Coleman <[email protected]>
8940337 to
8726d1f
Compare
Add support for DdlRel and WriteRel for Hive in Spark